Skip to content

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 3, 2024

This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were not many crates hitting this even when the lint was originally added.

This should get cratered, and I assume then it needs a t-compiler FCP. (t-compiler because this looks entirely like an implementation oversight -- for the vast majority of ABIs, we already have a hard error, but some were initially missed, and we are finally fixing that.)

Fixes #87678

@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2024

@bors try

@RalfJung RalfJung force-pushed the unsupported_calling_conventions branch from d933cf5 to 0c5a9b3 Compare September 3, 2024 16:56
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
…ions, r=

make unsupported_calling_conventions a hard error

This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang#86231 (comment)) even when the PR was landed.

This should get cratered, and I assume then it needs a t-compiler FCP.

Fixes rust-lang#88397
@bors
Copy link
Collaborator

bors commented Sep 3, 2024

⌛ Testing commit 0c5a9b3 with merge 498fce2...

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2024

Damn, this needs a change in the reference.^^

@ehuss
Copy link
Contributor

ehuss commented Sep 3, 2024

Damn, this needs a change in the reference.^^

Waiting 24 hours will also unblock, since beta week ends.

@bors
Copy link
Collaborator

bors commented Sep 3, 2024

☀️ Try build successful - checks-actions
Build commit: 498fce2 (498fce24f39c1ecbb0abe08824e72162da853341)

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2024

@craterbot check
The fact that crater is Linux-only is actually good here; on Windows the ABI keeps being accepted on all targets, only on other OSes does it get rejected.

@craterbot
Copy link
Collaborator

👌 Experiment pr-129935 created and queued.
🤖 Automatically detected try build 498fce2
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-129935 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129935 is completed!
📊 29 regressed and 3 fixed (508635 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@RalfJung
Copy link
Member Author

RalfJung commented Sep 9, 2024

6 crates on crates.io are affected, those all look like genuine regressions. They are all rarely used (the one with most downloads has 12k total downloads, that one also has not seen any updates in 8 years).

I have filed issues for the 4 crates that saw updates in the last 5 years, see the backlinks above.

@RalfJung RalfJung force-pushed the unsupported_calling_conventions branch from 0c5a9b3 to 2620cb2 Compare September 9, 2024 06:20
@RalfJung
Copy link
Member Author

RalfJung commented Sep 9, 2024

@fee1-dead I think this is then ready for review.

Does it need a t-compiler FCP because it is a breaking change?

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Oct 22, 2024

⌛ Testing commit de3cbf3 with merge 1de57a5...

@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #129935!

Tested on commit 1de57a5.
Direct link to PR: #129935

💔 reference on linux: test-pass → test-fail (cc @Havvy @matthewjasper @ehuss).

@bors
Copy link
Collaborator

bors commented Oct 22, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 1de57a5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 22, 2024
@bors bors merged commit 1de57a5 into rust-lang:master Oct 22, 2024
@rustbot rustbot added this to the 1.84.0 milestone Oct 22, 2024
@RalfJung
Copy link
Member Author

Reference should be fixed by rust-lang/reference#1600

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1de57a5): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Max RSS (memory usage)

Results (primary 1.7%, secondary -0.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.7% [1.7%, 1.7%] 1
Regressions ❌
(secondary)
1.6% [1.6%, 1.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) 1.7% [1.7%, 1.7%] 1

Cycles

Results (primary -2.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 781.081s -> 779.742s (-0.17%)
Artifact size: 333.69 MiB -> 333.70 MiB (0.00%)

@RalfJung RalfJung deleted the unsupported_calling_conventions branch October 22, 2024 07:23
@ehuss
Copy link
Contributor

ehuss commented Oct 22, 2024

@RalfJung There is an additional test that needs to be fixed. I posted rust-lang/reference#1659 to fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tracking issue for future-incompatibility lint unsupported_calling_conventions